Skip to content

feat: pass anon_id from dapp to wallet in SDKConnectV2 connection metadata for cross-side analytics correlation#28470

Open
ffmcgee725 wants to merge 9 commits intomainfrom
jc/WAPI-1382
Open

feat: pass anon_id from dapp to wallet in SDKConnectV2 connection metadata for cross-side analytics correlation#28470
ffmcgee725 wants to merge 9 commits intomainfrom
jc/WAPI-1382

Conversation

@ffmcgee725
Copy link
Copy Markdown
Member

@ffmcgee725 ffmcgee725 commented Apr 7, 2026

Description

Accepts and plumbs the dapp-provided anon_id through V2 (MWP) connections so wallet-side analytics events can be correlated with dapp-side events — matching the cross-side correlation that SDK v1 already provides.

  • Added optional analytics?: { anon_id: string } to the Metadata type
  • Added UUID validation for metadata.analytics.anon_id in the isConnectionRequest type guard
  • Plumbed anon_id into originatorInfo.anonId via HostApplicationAdapter.syncConnectionList()
  • Attached anon_id as a property on existing CONNECT_REQUEST_STARTED, CONNECT_REQUEST_COMPLETED, and CONNECT_REQUEST_CANCELLED analytics events (only when present, for V2 connections)
  • Added tests for validation and adapter plumbing

Related

Changelog

CHANGELOG entry: null

Related issues

Fixes:

Manual testing steps

Feature: my feature name

  Scenario: user [verb for user action]
    Given [describe expected initial app state]

    When user [verb for user action]
    Then [describe expected outcome]

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Note

Medium Risk
Touches SDKConnectV2 connection-request validation and connection/session plumbing, which could affect remote connection establishment if mishandled. Runtime impact is mostly telemetry-only (optional field, stripped when malformed) but analytics event payloads change across connect flows.

Overview
Adds optional metadata.analytics.remote_session_id to SDKConnectV2 connection requests, validating it as a UUID and stripping malformed analytics payloads instead of rejecting the connection.

Plumbs the value into originatorInfo.anonId via HostApplicationAdapter.syncConnectionList() and conditionally attaches it as remote_session_id on CONNECT_REQUEST_STARTED, CONNECT_REQUEST_CANCELLED, and CONNECT_REQUEST_COMPLETED events in PermissionApproval, AccountConnect, and MultichainAccountConnect.

Improves multichain connect analytics by deriving account_type via getAddressAccountType (fallback unknown) instead of the previous hardcoded value, and extends tests to cover the new analytics field and adapter behavior.

Reviewed by Cursor Bugbot for commit ec0f6c9. Bugbot is set up for automated code reviews on this repo. Configure here.

@ffmcgee725 ffmcgee725 requested review from a team as code owners April 7, 2026 13:54
@metamaskbot metamaskbot added the team-wallet-integrations Wallet Integrations team label Apr 7, 2026
@github-actions github-actions bot added size-M risk-medium Moderate testing recommended · Possible bug introduction risk labels Apr 7, 2026
@github-actions github-actions bot added risk-medium Moderate testing recommended · Possible bug introduction risk and removed risk-medium Moderate testing recommended · Possible bug introduction risk labels Apr 7, 2026
chain_id_list: chainIds,
referrer: channelIdOrHostname,
...getApiAnalyticsProperties(isMultichainRequest),
...(anonId ? { anon_id: anonId } : {}),
Copy link
Copy Markdown
Contributor

@adonesky1 adonesky1 Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe in all of these cases it should be added as a sensitive property as it is done in all of the SDKv1 instances?

.addSensitiveProperties({ anon_id: anonId })

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@github-actions github-actions bot added risk-medium Moderate testing recommended · Possible bug introduction risk and removed risk-medium Moderate testing recommended · Possible bug introduction risk labels Apr 8, 2026
...getApiAnalyticsProperties(isMultichainRequest),
});
if (anonId) {
eventBuilder.addSensitiveProperties({ anon_id: anonId });
Copy link
Copy Markdown
Member

@jiexi jiexi Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the id is already anonimized, is it still sensitive? If we trying to link mobile analytics identities to their dapp side identities, i'm not sure marking the property as sensitive would allow for that. If we only care about these specific events, then it should be fine

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@github-actions github-actions bot added risk-medium Moderate testing recommended · Possible bug introduction risk and removed risk-medium Moderate testing recommended · Possible bug introduction risk labels Apr 9, 2026
anon_id is a dapp-generated UUID for cross-side analytics correlation,
not a sensitive identifier. Marking it sensitive triggered the anonymous
event split (zeroed userId), which prevented linking dapp sessions to
wallet user activity. Per team consensus (Jiexi, Adam, Alex), this is
not actually sensitive data so it belongs in regular properties.
@github-actions
Copy link
Copy Markdown
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@github-actions github-actions bot added risk-medium Moderate testing recommended · Possible bug introduction risk and removed risk-medium Moderate testing recommended · Possible bug introduction risk labels Apr 10, 2026
accountType = getAddressAccountType(selectedCaipAccountIds[0]);
} catch {
accountType = 'unknown';
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated behavior change bundled without test coverage

Medium Severity

getAddressAccountType receives selectedCaipAccountIds[0], which is a CAIP account ID (e.g. a Solana eip155:… or solana:… format). Internally, the function parses the address then calls toFormattedAddress, which uses toChecksumAddress — an EVM-only operation that will throw for non-EVM chain addresses. While the try/catch gracefully defaults to 'unknown', the previous hard-coded 'multichain' value was semantically more accurate for this multichain flow. This behavior change (from 'multichain' to a per-address keyring lookup) is unrelated to the anon_id plumbing described in the PR and silently degrades the account_type analytics property to 'unknown' for all non-EVM account selections.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 3771262. Configure here.

@github-actions github-actions bot removed the risk-medium Moderate testing recommended · Possible bug introduction risk label Apr 10, 2026
@github-actions github-actions bot added the risk-medium Moderate testing recommended · Possible bug introduction risk label Apr 10, 2026
The property was an opaque dapp-generated UUID used for cross-side
analytics correlation — "remote_session_id" better describes its
purpose. Updated the Metadata type, connection-request validation,
host-application adapter mapping, event properties on Connect Request
events, and all related tests.

Aligns with the segment-schema definitions which already use
remote_session_id.
@github-actions github-actions bot added risk-medium Moderate testing recommended · Possible bug introduction risk and removed risk-medium Moderate testing recommended · Possible bug introduction risk labels Apr 10, 2026
@github-actions
Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeNetworkAbstractions, SmokeMultiChainAPI, SmokeNetworkExpansion, SmokeConfirmations, FlaskBuildTests
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: medium
  • AI Confidence: 82%
click to see 🤖 AI reasoning details

E2E Test Selection:

The PR introduces analytics telemetry enhancements to SDK V2 connection flows:

  1. SDKConnectV2 metadata/connection-request changes: Added optional analytics.remote_session_id field to the Metadata type, with graceful validation in isConnectionRequest that strips malformed analytics data rather than rejecting connections. This is a safe, additive change but touches the core connection request validation path.

  2. host-application-adapter.ts: Maps analytics.remote_session_id to anonId in originatorInfo when syncing SDK V2 connection lists. This affects how connections are stored in Redux state.

  3. AccountConnect.tsx & MultichainAccountConnect.tsx: Both now use useSDKV2Connection to retrieve anonId and include remote_session_id in analytics events. MultichainAccountConnect also fixes account_type from hardcoded 'multichain' to the actual account type via getAddressAccountType. These are the primary dApp connection UI components.

  4. PermissionApproval.tsx: Adds anonId to the CONNECT_REQUEST_COMPLETED analytics event.

Tags selected:

  • SmokeNetworkAbstractions: Directly covers dApp chain permission flows, network selector, and chain permission system - the AccountConnect and PermissionApproval components are central to these flows.
  • SmokeMultiChainAPI: Tests CAIP-25 multi-chain session API and wallet_createSession flows - MultichainAccountConnect is directly involved in these flows.
  • SmokeNetworkExpansion: Tests multi-chain provider architecture and Solana wallet standard - MultichainAccountConnect handles multi-chain connections including non-EVM chains.
  • SmokeConfirmations: Per tag descriptions, SmokeNetworkAbstractions and SmokeMultiChainAPI integrations require SmokeConfirmations. Also, PermissionApproval is part of the confirmation/approval flow.
  • FlaskBuildTests: Snaps use dApp connection flows (AccountConnect, PermissionApproval) for snap installation and permission grants - these components are in the critical path for snap connections.

Performance tests: Not selected - changes are purely analytics/telemetry additions (optional spread of anonId into event properties). No UI rendering changes, no new data fetching, no state management changes that would affect performance.

Performance Test Selection:
Changes are purely analytics/telemetry enhancements - adding optional remote_session_id to event properties via a conditional spread. No UI rendering changes, no new data fetching, no state management changes, no list rendering modifications. The anonId lookup is a simple Redux selector call via useSDKV2Connection hook which was already in use. No performance impact expected.

View GitHub Actions results

adonesky1 added a commit that referenced this pull request Apr 10, 2026
…rties

Aligns with the decision in PR #28470 — remote_session_id is a
dapp-generated UUID for cross-side analytics correlation, not sensitive
data. Keeping it in sensitiveProperties triggered the anonymous event
split which prevented linking dapp sessions to wallet user activity.
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit ec0f6c9. Configure here.

!isUUID(metadata.analytics.remote_session_id)
) {
delete (metadata as unknown as Record<string, unknown>).analytics;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type guard mutates its input argument silently

Low Severity

The isConnectionRequest type guard mutates its input by deleting metadata.analytics when malformed. Type guards are conventionally pure predicate functions — callers don't expect them to have side effects on the checked object. Since data is passed by reference, the deletion silently alters the caller's object. Extracting the sanitization into a separate step (e.g., a sanitizeConnectionRequest function called after the guard) would make the mutation explicit and keep the type guard free of side effects.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit ec0f6c9. Configure here.

@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
78.4% Coverage on New Code (required ≥ 80%)
8.3% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@github-actions
Copy link
Copy Markdown
Contributor

E2E Fixture Validation — Schema is up to date
17 value mismatches detected (expected — fixture represents an existing user).
View details

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk-medium Moderate testing recommended · Possible bug introduction risk size-M team-wallet-integrations Wallet Integrations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants